docs: Add SKILL.md for designing benchmark - #25285
Conversation
|
|
||
| ## Rust microbenchmarks | ||
|
|
||
| TODO |
There was a problem hiding this comment.
I don't have ideas specific to Rust microbenchmarks; maybe others do and we can add them later.
There was a problem hiding this comment.
It is better to remove this section entirely if we do not have the content ready. Otherwise, it may confuse agents.
There was a problem hiding this comment.
I agree. Removed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25285 +/- ##
==========================================
+ Coverage 81.90% 82.28% +0.37%
==========================================
Files 1134 1137 +3
Lines 425217 430211 +4994
Branches 425217 430211 +4994
==========================================
+ Hits 348289 353995 +5706
+ Misses 56288 54790 -1498
- Partials 20640 21426 +786 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| 1. **Isolate the operator being measured in microbenchmarks.** | ||
|
|
||
| When a microbenchmark targets a specific operator, keep the work done by |
There was a problem hiding this comment.
Say when criterion benches are still the right tool. The repo has about 180 criterion bench files. "Prefer SQL benchmarks even for microbenchmarks" with no exceptions will clash with reviews of PRs that touch hashing, row conversion or array kernels, where SQL setup would drown out the code being measured. One sentence would cover it, e.g.: "Use a criterion bench only when the code has no SQL-reachable path, or when per-row costs are too small to see through a query.
There was a problem hiding this comment.
Clear up the tension between the two main rules. "Benchmark at a higher level" and "isolate the operator" can read as opposite advice. Suggested wording: go in through the highest-level interface (SQL), but keep everything around the operator as cheap as possible.
There was a problem hiding this comment.
Mention expect_plan. Every nlj query has expect_plan NestedLoopJoinExec. It matters a lot: if an optimizer change turns the NLJ into a hash join, the benchmark keeps "passing" while measuring the wrong operator. I'd make it its own rule: assert the plan shape you mean to measure.
9bf11b7 to
53da0c2
Compare
|
@jayzhan211 @Xuanwo Thank you for the review. |
4 participants
Which issue does this PR close?
Rationale for this change
Here is something I had in mind when designing or reviewing benchmarks. Summarize it as a doc for future reference.
What changes are included in this PR?
Add a skill for designing new benchmarks.
What is the testing strategy for this PR?
Are there any user-facing changes?